fix: 20 - исправлена ошибка аутентификации#21
Conversation
📝 WalkthroughWalkthroughIntroduces pluggable auth providers, a new HttpClient, centralized logging, cache/path managers, file discovery and platform utils, migrates CI and scripts to pnpm, replaces request module, updates ESLint config, and adds tests; multiple modules refactored to use the new abstractions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Factory as AuthProviderFactory
participant Provider as AuthProvider (Token/Form)
participant HttpClient as HttpClient
participant Server as 1C Server
Client->>Factory: create(config)
Factory-->>Provider: instantiate(username,password)
Client->>Provider: authenticate(httpClient)
Provider->>HttpClient: request login/ticket (GET/POST)
HttpClient->>Server: send HTTP request
Server-->>HttpClient: response (HTML / token / redirect)
HttpClient-->>Provider: return response
Provider->>HttpClient: verify (GET releases or follow ticket)
HttpClient->>Server: follow-up request
Server-->>HttpClient: final response
HttpClient-->>Provider: final status
Provider-->>Client: authentication complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/edt.ts (1)
55-71:⚠️ Potential issue | 🟠 MajorGuard against empty installer matches before exec.
findFilescan return an empty array;files[0]will beundefinedand the install step will fail with a confusing error. Add an explicit check and fail early with a clear message.🛠️ Proposed fix
- const files = await findFiles(patterns.join('\n')) - - logger.info(`finded ${files}`) + const files = await findFiles(patterns.join('\n')) + if (files.length === 0) { + core.setFailed(`Installer not found for patterns: ${patterns.join(', ')}`) + return + } + + logger.info(`found ${files.join(', ')}`)src/tools/platform83.ts (1)
60-66:⚠️ Potential issue | 🟠 MajorGuard against missing installer before
exec.
findFiles(...)can return an empty array;files[0]becomesundefined, leading to an opaque exec failure. Fail fast with an explicit error.🛠️ Proposed fix
- const files = await findFiles(`${path}/**/${installerPattern}*`) - logger.info(`found ${files}`) + const files = await findFiles(`${path}/**/${installerPattern}*`) + if (files.length === 0) { + core.setFailed(`Installer not found for pattern: ${installerPattern}`) + return + } + logger.info(`found ${files}`)
🤖 Fix all issues with AI agents
In `@__tests__/authProviders.test.ts`:
- Around line 72-77: The test's error assertion is too strict for
TokenAuthProvider; update the catch block in the authProviders.test.ts test to
assert more flexibly—for example check that error is defined and that (error as
Error).message matches a case-insensitive substring like "auth" or "failed"
(e.g., use a regex /auth|failed/i) or simply assert it's an instance of Error—so
modify the expect((error as
Error).message.toLowerCase()).toContain('authentication') to a looser check that
will accept messages like "Auth failed with status ...".
- Around line 81-87: The test invoking FormAuthProvider.authenticate should not
run when real credentials are absent; update the test in authProviders.test.ts
to guard on the hasCredentials flag (or use test.skip) so it only executes when
credentials are provided, or alternatively supply mocked credentials/http
behavior; specifically modify the it block that creates FormAuthProvider(login,
password) and HttpClient and calls provider.authenticate(httpClient) to
early-skip when hasCredentials is false (or substitute a mocked
HttpClient/credentials) to avoid CI failures.
In `@__tests__/downloader.test.ts`:
- Around line 5-12: The test creates a Client with login/password derived from
process.env which can be empty and causes the Client constructor to throw before
the try/catch; modify the test to either provide safe defaults or skip when
credentials are missing: check the login/password variables and if empty call
test.skip for the "auth with token provider" case, or set non-empty placeholder
credentials before constructing new Client(login, password, { preferredProvider:
AuthProviderType.TOKEN }) so the Client constructor never throws; reference the
Client constructor, the login/password variables and the test named "auth with
token provider" when applying the change.
In `@package.json`:
- Line 6: Update the package.json metadata to reflect that this repo is a GitHub
Action by setting the "private" field to true; open package.json, locate the
top-level "private" property and change its value from false to true so the
project is not treated as a publishable npm package.
In `@src/onegetjs/auth/tokenAuthProvider.ts`:
- Around line 59-63: Validate that the parsed response `data` contains a
non-empty `ticket` before building the redirect URL: in the token exchange logic
in tokenAuthProvider.ts (where `response`, `data`, and `LOGIN_URL` are used)
check `data && data.ticket` and if missing throw or return a clear error (or
reject) indicating the missing ticket instead of returning
`${LOGIN_URL}/ticket/auth?token=undefined`; this ensures the caller receives a
deterministic error and prevents constructing an invalid URL.
In `@src/onegetjs/downloader.ts`:
- Around line 91-95: The promise that pipes streamResponse.data into destination
is missing an error handler for the write stream; update the code in
downloader.ts (around the pipe logic) to attach destination.on('error', reject)
in addition to streamResponse.data.on('error', reject) so write failures (e.g.,
disk full, permission denied) will reject the promise; optionally ensure you
also remove/cleanup listeners after resolve/reject to avoid leaks in the same
block where streamResponse.data.pipe(destination) is called.
In `@src/onegetjs/httpClient.ts`:
- Around line 84-87: The current isJson check in httpClient.ts is too strict
because it expects an exact Content-Type value; update the logic that computes
isJson (the const isJson) to detect JSON when data is an object and the
Content-Type header (from config.headers, checking both 'Content-Type' and
'content-type' keys) startsWith or includes 'application/json' (e.g., using
.startsWith('application/json') or indexOf >= 0) to accept charsets like ';
charset=utf-8'; ensure you still handle the case when config.headers is
undefined.
In `@src/onegetjs/index.ts`:
- Around line 38-40: The user-facing error message in the conditional that
checks files.length === 0 should be corrected from the typo "Found't files" to a
clear message like "No files found"; update the call to this.error in the block
that references files and artifactFilter (e.g., the this.error(...) inside the
files.length === 0 branch) to use a clearer string such as `No files found for
version ${JSON.stringify(artifactFilter)}` so users see a readable, accurate
message.
- Around line 123-126: The error() method currently throws a raw string which
breaks stack traces and consistency; update the method (error(message: string):
void) to still call logger.error(message) but throw a proper Error object
instead (e.g., throw new Error(message)) so downstream handlers receive Error
instances and stack traces are preserved.
In `@src/onegetjs/parse.ts`:
- Around line 12-31: The fallback loop over altSelectors (using
root.querySelectorAll and returning altCells.map(...) as Version) is too
permissive and can return unrelated links; update the logic so that before
returning you filter altCells to only items with a non-null href
(cell.getAttribute('href') !== null) and a name that matches a version-shaped
pattern (e.g., semantic version or presence of digits/lemmas like
/\b\d+(\.\d+)+\b/ or 'ver='), and only return the mapped Version array if the
filtered array is non-empty; otherwise continue to the next selector so only
validated version-like links are emitted.
🧹 Nitpick comments (10)
__tests__/parse.test.ts (1)
3-24: Add a test that exercises the fallback selectors.
Current test only covers the primary selector path; the new fallback path isn’t verified.Example test addition
describe('parse.ts', () => { it('should parse versions from HTML', () => { const html = ` <table> <tr> <td class="versionColumn"> <a href="/version/8.3.25.1286">8.3.25.1286</a> </td> </tr> <tr> <td class="versionColumn"> <a href="/version/8.3.24.1500">8.3.24.1500</a> </td> </tr> </table> ` const result = versions(html) expect(result).toHaveLength(2) expect(result[0].name).toBe('8.3.25.1286') expect(result[1].name).toBe('8.3.24.1500') }) + + it('should parse versions using fallback selectors', () => { + const html = ` + <div> + <a class="version-link" href="/version/8.3.25.1286">8.3.25.1286</a> + <a class="version-link" href="/version/8.3.24.1500">8.3.24.1500</a> + </div> + ` + const result = versions(html) + expect(result).toHaveLength(2) + }) }).github/workflows/linter.yml (1)
35-38: AddpackageManagerfield to package.json to pin pnpm version.
Usingversion: latestin the pnpm setup action can introduce non-reproducible builds. Add"packageManager": "pnpm@<version>"to package.json, then update the workflow to use that pinned version instead oflatest.eslint.config.mjs (2)
8-36: Scope Jest globals to test files only.Applying Jest globals to all files can hide accidental test-only usage in production code. Consider a separate test-only config block.
♻️ Example structure
{ files: ['**/*.ts', '**/*.tsx', '**/*.js', '**/*.jsx'], languageOptions: { parser: typescriptParser, parserOptions: { ecmaVersion: 2023, sourceType: 'module' }, globals: { process: 'readonly', console: 'readonly', Buffer: 'readonly', __dirname: 'readonly', __filename: 'readonly', global: 'readonly', module: 'readonly', require: 'readonly', exports: 'readonly', URL: 'readonly', URLSearchParams: 'readonly', - describe: 'readonly', - it: 'readonly', - test: 'readonly', - expect: 'readonly', - beforeEach: 'readonly', - afterEach: 'readonly', - beforeAll: 'readonly', - afterAll: 'readonly', - jest: 'readonly' } }, plugins: { '@typescript-eslint': typescript }, rules: { 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': 'error', '@typescript-eslint/no-explicit-any': 'warn' } }, + { + files: ['**/*.test.*', '**/__tests__/**'], + languageOptions: { + globals: { + describe: 'readonly', + it: 'readonly', + test: 'readonly', + expect: 'readonly', + beforeEach: 'readonly', + afterEach: 'readonly', + beforeAll: 'readonly', + afterAll: 'readonly', + jest: 'readonly' + } + } + },
48-48: Ignore all JSON files if that’s the intent.
'*.json'only matches JSON at the repo root; nested JSON will still be linted.✅ Suggested tweak
- ignores: ['node_modules/**', 'dist/**', 'coverage/**', '*.json'] + ignores: ['node_modules/**', 'dist/**', 'coverage/**', '**/*.json']src/utils.ts (1)
42-47: Use warning level for cache restore failures.
logger.info('[warning]...')hides severity compared to other warnings in this file.🔧 Suggested change
- logger.info(`[warning]${message}`) + logger.warning(message)src/tools/cacheManager.ts (1)
25-39: Log cache save failures at warning/error level with key context.These errors are easy to miss at info level, and the key is useful for diagnostics.
🛠️ Suggested change
async saveInstaller(key: string, paths: string[]): Promise<void> { try { await cache.saveCache(paths, key) } catch (error) { - if (error instanceof Error) logger.info(error.message) + if (error instanceof Error) { + logger.warning(`Cache save failed for ${key}: ${error.message}`) + } } } async saveInstalled(key: string, paths: string[]): Promise<void> { try { logger.info(`Trying to save: ${paths.toString()}`) await cache.saveCache(paths, key) } catch (error) { - if (error instanceof Error) logger.info(error.message) + if (error instanceof Error) { + logger.warning(`Cache save failed for ${key}: ${error.message}`) + } } }src/onegetjs/httpClient.ts (1)
48-83: Replaceanyin the public API to satisfy lint and improve type safety.The current signatures trigger
Unexpected anywarnings. Consider narrowing types to minimal, permissive shapes.♻️ Example refactor
- async get( - url: string, - options?: any - ): Promise<{ data: any; request: any; status: number; headers?: any }> { + async get( + url: string, + options?: { isStream?: boolean } & Record<string, unknown> + ): Promise<{ + data: unknown + request: { res: { responseUrl: string } } + status: number + headers?: Record<string, string> + }> { @@ - async post( - url: string, - data: any, - config: any = {} - ): Promise<{ data: any; request: any; status: number; headers?: any }> { + async post( + url: string, + data: unknown, + config: { headers?: Record<string, string>; followRedirect?: boolean } = {} + ): Promise<{ + data: unknown + request: { res: { responseUrl: string } } + status: number + headers?: Record<string, string> + }> {src/tools/onecTool.ts (1)
26-28: Hardcoded/tmp/path may not work on Windows.The method returns a Unix-specific path while the class includes platform detection methods. If cross-platform support is intended, consider using a platform-agnostic temp directory.
♻️ Suggested platform-agnostic approach
+import * as os from 'os' +import * as path from 'path' getInstallersPath(): string { - return `/tmp/${this.INSTALLER_CACHE_PRIMARY_KEY}` + return path.join(os.tmpdir(), this.INSTALLER_CACHE_PRIMARY_KEY) }If this code only runs in Linux CI environments, the hardcoded path is acceptable and this can be ignored.
src/onegetjs/downloader.ts (2)
43-45: Consider adding explicit return type to address ESLint warning.The static analysis tool flagged the
anyreturn type. SinceHttpClient.get()returns a known structure, you can type this more precisely.♻️ Proposed type improvement
-async get(url: string): Promise<any> { - return this.httpClient.get(url) -} +async get( + url: string +): Promise<{ data: any; request: any; status: number; headers?: any }> { + return this.httpClient.get(url) +}Alternatively, define a shared
HttpResponseinterface inhttpClient.tsand reuse it here.
47-56: Consider typing theheadersparameter to address ESLint warning.The static analysis tool flagged
anyon the headers parameter.♻️ Proposed type improvement
-extractFileName(url: string, headers?: any): string | undefined { +extractFileName( + url: string, + headers?: Record<string, string | string[] | undefined> +): string | undefined {
| } catch (error) { | ||
| // Ожидаем ошибку, но проверяем что это правильная ошибка аутентификации | ||
| expect(error).toBeDefined() | ||
| expect((error as Error).message.toLowerCase()).toContain( | ||
| 'authentication' | ||
| ) |
There was a problem hiding this comment.
Relax the TokenAuthProvider error message assertion.
TokenAuthProvider can throw errors like “Auth failed with status ...”, which won’t include “authentication”, so the assertion is brittle.
🧪 Suggested fix
- expect((error as Error).message.toLowerCase()).toContain(
- 'authentication'
- )
+ expect((error as Error).message.toLowerCase()).toMatch(/auth|login|token/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| // Ожидаем ошибку, но проверяем что это правильная ошибка аутентификации | |
| expect(error).toBeDefined() | |
| expect((error as Error).message.toLowerCase()).toContain( | |
| 'authentication' | |
| ) | |
| } catch (error) { | |
| // Ожидаем ошибку, но проверяем что это правильная ошибка аутентификации | |
| expect(error).toBeDefined() | |
| expect((error as Error).message.toLowerCase()).toMatch(/auth|login|token/) | |
| } |
🤖 Prompt for AI Agents
In `@__tests__/authProviders.test.ts` around lines 72 - 77, The test's error
assertion is too strict for TokenAuthProvider; update the catch block in the
authProviders.test.ts test to assert more flexibly—for example check that error
is defined and that (error as Error).message matches a case-insensitive
substring like "auth" or "failed" (e.g., use a regex /auth|failed/i) or simply
assert it's an instance of Error—so modify the expect((error as
Error).message.toLowerCase()).toContain('authentication') to a looser check that
will accept messages like "Auth failed with status ...".
| it('FormAuthProvider should attempt authentication', async () => { | ||
| const provider = new FormAuthProvider(login, password) | ||
| const httpClient = new HttpClient() | ||
| await provider.authenticate(httpClient) | ||
| // Если дошли сюда - аутентификация прошла | ||
| expect(true).toBe(true) | ||
| }, 30000) |
There was a problem hiding this comment.
Skip or handle FormAuthProvider auth when credentials are missing.
This test runs even when hasCredentials is false (defaults to 'test'), so it will fail in CI without real credentials.
🧪 Suggested fix
- it('FormAuthProvider should attempt authentication', async () => {
- const provider = new FormAuthProvider(login, password)
- const httpClient = new HttpClient()
- await provider.authenticate(httpClient)
- // Если дошли сюда - аутентификация прошла
- expect(true).toBe(true)
- }, 30000)
+ it('FormAuthProvider should attempt authentication', async () => {
+ if (!hasCredentials) {
+ console.log('Skipping FormAuthProvider test - no credentials')
+ return
+ }
+ const provider = new FormAuthProvider(login, password)
+ const httpClient = new HttpClient()
+ await provider.authenticate(httpClient)
+ // Если дошли сюда - аутентификация прошла
+ expect(true).toBe(true)
+ }, 30000)🤖 Prompt for AI Agents
In `@__tests__/authProviders.test.ts` around lines 81 - 87, The test invoking
FormAuthProvider.authenticate should not run when real credentials are absent;
update the test in authProviders.test.ts to guard on the hasCredentials flag (or
use test.skip) so it only executes when credentials are provided, or
alternatively supply mocked credentials/http behavior; specifically modify the
it block that creates FormAuthProvider(login, password) and HttpClient and calls
provider.authenticate(httpClient) to early-skip when hasCredentials is false (or
substitute a mocked HttpClient/credentials) to avoid CI failures.
| const login = process.env.ONEC_USERNAME ?? '' | ||
| const password = process.env.ONEC_PASSWORD ?? '' | ||
| const client = new Client(login, password) | ||
|
|
||
| it('auth', async () => { | ||
| await client.auth() | ||
| it('auth with token provider', async () => { | ||
| const client = new Client(login, password, { | ||
| preferredProvider: AuthProviderType.TOKEN | ||
| }) | ||
| try { |
There was a problem hiding this comment.
Avoid constructor throw when env credentials are missing.
Client throws if login/password are empty; that happens before the try/catch, so these tests will fail in environments without secrets. Consider defaulting to non-empty placeholders or skipping when env vars are absent.
✅ Suggested fix (placeholders)
- const login = process.env.ONEC_USERNAME ?? ''
- const password = process.env.ONEC_PASSWORD ?? ''
+ const login = process.env.ONEC_USERNAME ?? 'test'
+ const password = process.env.ONEC_PASSWORD ?? 'test'🤖 Prompt for AI Agents
In `@__tests__/downloader.test.ts` around lines 5 - 12, The test creates a Client
with login/password derived from process.env which can be empty and causes the
Client constructor to throw before the try/catch; modify the test to either
provide safe defaults or skip when credentials are missing: check the
login/password variables and if empty call test.skip for the "auth with token
provider" case, or set non-empty placeholder credentials before constructing new
Client(login, password, { preferredProvider: AuthProviderType.TOKEN }) so the
Client constructor never throws; reference the Client constructor, the
login/password variables and the test named "auth with token provider" when
applying the change.
| await new Promise((resolve, reject) => { | ||
| response.body.pipe(destination) | ||
| response.body.on('error', reject) | ||
| destination.on('finish', resolve) | ||
| streamResponse.data.pipe(destination) | ||
| streamResponse.data.on('error', reject) | ||
| destination.on('finish', () => resolve(undefined)) | ||
| }) |
There was a problem hiding this comment.
Missing error handler for the write stream.
The code handles errors from the read stream (streamResponse.data.on('error', reject)) but doesn't handle errors from the write stream (destination). Write failures (e.g., disk full, permission denied) would not reject the promise.
🛡️ Proposed fix to handle write stream errors
await new Promise((resolve, reject) => {
streamResponse.data.pipe(destination)
streamResponse.data.on('error', reject)
+ destination.on('error', reject)
destination.on('finish', () => resolve(undefined))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise((resolve, reject) => { | |
| response.body.pipe(destination) | |
| response.body.on('error', reject) | |
| destination.on('finish', resolve) | |
| streamResponse.data.pipe(destination) | |
| streamResponse.data.on('error', reject) | |
| destination.on('finish', () => resolve(undefined)) | |
| }) | |
| await new Promise((resolve, reject) => { | |
| streamResponse.data.pipe(destination) | |
| streamResponse.data.on('error', reject) | |
| destination.on('error', reject) | |
| destination.on('finish', () => resolve(undefined)) | |
| }) |
🤖 Prompt for AI Agents
In `@src/onegetjs/downloader.ts` around lines 91 - 95, The promise that pipes
streamResponse.data into destination is missing an error handler for the write
stream; update the code in downloader.ts (around the pipe logic) to attach
destination.on('error', reject) in addition to streamResponse.data.on('error',
reject) so write failures (e.g., disk full, permission denied) will reject the
promise; optionally ensure you also remove/cleanup listeners after
resolve/reject to avoid leaks in the same block where
streamResponse.data.pipe(destination) is called.
| const isJson = | ||
| typeof data === 'object' && | ||
| config.headers?.['Content-Type'] === 'application/json' | ||
|
|
There was a problem hiding this comment.
Relax JSON Content-Type matching.
Common headers include a charset (e.g., application/json; charset=utf-8), which fails the strict equality check.
🛠️ Proposed fix
- const isJson =
- typeof data === 'object' &&
- config.headers?.['Content-Type'] === 'application/json'
+ const contentType = (config.headers?.['Content-Type'] || '').toLowerCase()
+ const isJson =
+ typeof data === 'object' && contentType.startsWith('application/json')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isJson = | |
| typeof data === 'object' && | |
| config.headers?.['Content-Type'] === 'application/json' | |
| const contentType = (config.headers?.['Content-Type'] || '').toLowerCase() | |
| const isJson = | |
| typeof data === 'object' && contentType.startsWith('application/json') | |
🤖 Prompt for AI Agents
In `@src/onegetjs/httpClient.ts` around lines 84 - 87, The current isJson check in
httpClient.ts is too strict because it expects an exact Content-Type value;
update the logic that computes isJson (the const isJson) to detect JSON when
data is an object and the Content-Type header (from config.headers, checking
both 'Content-Type' and 'content-type' keys) startsWith or includes
'application/json' (e.g., using .startsWith('application/json') or indexOf >= 0)
to accept charsets like '; charset=utf-8'; ensure you still handle the case when
config.headers is undefined.
| // Если основной селектор не работает, попробуем альтернативные | ||
| if (cells.length === 0) { | ||
| const altSelectors = [ | ||
| 'a[href*="ver="]', | ||
| '.version-link', | ||
| 'td a', | ||
| '.versionColumn a' | ||
| ] | ||
|
|
||
| for (const selector of altSelectors) { | ||
| const altCells = root.querySelectorAll(selector) | ||
| if (altCells.length > 0) { | ||
| return altCells.map( | ||
| cell => | ||
| ({ | ||
| name: cell.text.trim(), | ||
| url: cell.getAttribute('href') | ||
| }) as Version | ||
| ) | ||
| } |
There was a problem hiding this comment.
Guard fallback matches to avoid unrelated links.
The fallback uses broad selectors (e.g., td a) and returns the first match set without validating href or version-like text. That can yield incorrect versions under fallback. Consider filtering by non-null href and a version-shaped name before returning.
Proposed hardening
const PROJECT_VERSIONS_SELECTOR = 'td.versionColumn>a'
const RELEASE_FILES_SELECTOR = '.files-container .formLine a'
const DOWNLOAD_LINK_SELECTOR = '.downloadDist a'
+const VERSION_NAME_RE = /^\d+(\.\d+){3}$/
export function versions(content: string): Version[] {
const root = parse(content)
const cells = root.querySelectorAll(PROJECT_VERSIONS_SELECTOR)
// Если основной селектор не работает, попробуем альтернативные
if (cells.length === 0) {
const altSelectors = [
'a[href*="ver="]',
'.version-link',
'td a',
'.versionColumn a'
]
for (const selector of altSelectors) {
const altCells = root.querySelectorAll(selector)
- if (altCells.length > 0) {
- return altCells.map(
- cell =>
- ({
- name: cell.text.trim(),
- url: cell.getAttribute('href')
- }) as Version
- )
- }
+ const versions = altCells
+ .map(cell => ({
+ name: cell.text.trim(),
+ url: cell.getAttribute('href')
+ }))
+ .filter(
+ (v): v is { name: string; url: string } =>
+ Boolean(v.url) && VERSION_NAME_RE.test(v.name)
+ )
+ if (versions.length > 0) {
+ return versions.map(v => v as Version)
+ }
}
}🤖 Prompt for AI Agents
In `@src/onegetjs/parse.ts` around lines 12 - 31, The fallback loop over
altSelectors (using root.querySelectorAll and returning altCells.map(...) as
Version) is too permissive and can return unrelated links; update the logic so
that before returning you filter altCells to only items with a non-null href
(cell.getAttribute('href') !== null) and a name that matches a version-shaped
pattern (e.g., semantic version or presence of digits/lemmas like
/\b\d+(\.\d+)+\b/ or 'ver='), and only return the mapped Version array if the
filtered array is non-empty; otherwise continue to the next selector so only
validated version-like links are emitted.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/onegetjs/httpClient.ts`:
- Around line 52-56: When options.isStream is true, the current code calls
this.client.stream.get(url, { timeout: { request: 300000 } }) and drops all
other passed options; update the stream branch to merge the incoming options
object into the request while overriding/ensuring the extended timeout (e.g.,
create a mergedOptions = { ...options, timeout: { request: 300000 }, /* preserve
responseType/headers/searchParams */ } and pass mergedOptions to
this.client.stream.get) so responseType, headers, searchParams, etc. are
forwarded while guaranteeing the 5-minute timeout.
- Around line 50-83: Update the HttpClient method signatures to remove all uses
of any: import got's OptionsInit and define a generic HttpResponse<T> type
(data: T, request: { res: { responseUrl?: string } }, status: number, headers?:
Record<string,string|string[]>), then change get to async get<T = unknown>(url:
string, options?: OptionsInit & { isStream?: boolean }):
Promise<HttpResponse<T>> and post to async post<T = unknown>(url: string, data:
unknown, config: OptionsInit = {}): Promise<HttpResponse<T>>; inside get/post
return types use T for response body and use proper header typing; also change
any catch clauses (e.g., catch (error: any)) to catch (error: unknown) and
narrow or rethrow appropriately.
In `@src/onegetjs/index.ts`:
- Line 56: The log message "Don't found links for file ${file.name}" is
ungrammatical; update the logger.error call (the logger.error invocation in
src/onegetjs/index.ts that references file.name) to a clear, correct message
such as "No links found for file ${file.name}" or "No links found in file
${file.name}" so the message reads properly and includes the file name for
context.
| options?: any | ||
| ): Promise<{ data: any; request: any; status: number; headers?: any }> { | ||
| if (options?.isStream) { | ||
| // For streams, return the stream directly with extended timeout | ||
| const stream = this.client.stream.get(url, { | ||
| timeout: { request: 300000 } // 5 minutes for large files | ||
| }) | ||
| return new Promise((resolve, reject) => { | ||
| stream.on('response', (response: Response) => { | ||
| resolve({ | ||
| data: stream, | ||
| request: { res: { responseUrl: response.url } }, | ||
| status: response.statusCode, | ||
| headers: response.headers | ||
| }) | ||
| }) | ||
| stream.on('error', reject) | ||
| }) | ||
| } | ||
|
|
||
| const response = await this.client.get(url, options) | ||
| return { | ||
| data: response.body, | ||
| request: { res: { responseUrl: response.url } }, | ||
| status: response.statusCode, | ||
| headers: response.headers | ||
| } | ||
| } | ||
|
|
||
| async post( | ||
| url: string, | ||
| data: any, | ||
| config: any = {} | ||
| ): Promise<{ data: any; request: any; status: number; headers?: any }> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant sections
cat -n src/onegetjs/httpClient.ts | head -100Repository: 1CDevFlow/onec-setup-action
Length of output: 3710
🏁 Script executed:
# Search for all `any` occurrences in the file with context
rg -n '\bany\b' src/onegetjs/httpClient.ts -B2 -A2Repository: 1CDevFlow/onec-setup-action
Length of output: 780
🏁 Script executed:
# Check the full file for all `any` occurrences
rg -n '\bany\b' src/onegetjs/httpClient.tsRepository: 1CDevFlow/onec-setup-action
Length of output: 320
🏁 Script executed:
# Check if there are other methods in the file that might have `any`
cat -n src/onegetjs/httpClient.ts | tail -50Repository: 1CDevFlow/onec-setup-action
Length of output: 1878
🌐 Web query:
got.js types Parameters Got get post method signature TypeScript
💡 Result:
In current got (v14.x), .get and .post are HTTP aliases on the got instance, and they’re both typed as the same overloaded function type: GotRequestFunction. The instance type is effectively:
type Got = … & Record<HTTPAlias, GotRequestFunction<…>> & GotRequestFunction<…>(sogot.get,got.post, etc. areGotRequestFunction) [1]type HTTPAlias = 'get' | 'post' | 'put' | 'patch' | 'head' | 'delete'[1]
The TypeScript call signatures (simplified)
GotRequestFunction is heavily overloaded. Some key overloads are: [2]
type GotRequestFunction = {
(url: string | URL): CancelableRequest<Response<DefaultResponseBodyType>> | CancelableRequest<DefaultResponseBodyType>;
(url: string | URL, options?: OptionsOfTextResponseBody): CancelableRequest<Response<string>> | CancelableRequest<string>;
<T>(url: string | URL, options?: OptionsOfJSONResponseBody): CancelableRequest<Response<T>> | CancelableRequest<T>;
(url: string | URL, options?: OptionsOfBufferResponseBody): CancelableRequest<Response<Buffer>> | CancelableRequest<Buffer>;
(options: { url: string | URL }): ...;
(options: OptionsOfTextResponseBody): ...;
<T>(options: OptionsOfJSONResponseBody): ...;
// Stream mode overloads:
(url: string | URL, options?: OptionsInit & { isStream: true }): Request;
(options: OptionsInit & { isStream: true }): Request;
// Fallback:
(url: string | URL, options?: OptionsInit): CancelableRequest | Request;
(options: OptionsInit): CancelableRequest | Request;
};So the practical method signatures are:
got.get(url: string | URL, options?: OptionsInit): CancelableRequest | Request
got.post(url: string | URL, options?: OptionsInit): CancelableRequest | Request…plus the more specific overloads above for .json<T>() / responseType-driven typing, etc. [2]
Why Parameters<typeof got.get> doesn’t give “the” signature you expect
Parameters<> can’t extract all overload parameter lists; it effectively “sees” only one signature (and overload/implementation-signature behavior is often surprising). TypeScript explains this general overload pitfall in its FAQ. [3]
If you tell me your exact goal (e.g., “I want a type for (url, options?) only” vs “I want all overload tuples”), I can give the cleanest TS helper type for that.
Replace any in HttpClient signatures with proper got library types.
ESLint flags Unexpected any for options, data, config, and return shapes. Use got's exported option types and a generic response wrapper instead of relying on Parameters<> extraction, which is unreliable for overloaded function signatures.
Consider importing OptionsInit from got directly and defining:
type HttpResponse<T = unknown> = {
data: T
request: { res: { responseUrl?: string } }
status: number
headers?: Record<string, string | string[]>
}
type GetOptions = OptionsInit & { isStream?: boolean }
type PostOptions = OptionsInit
async get<T = unknown>(
url: string,
options?: GetOptions
): Promise<HttpResponse<T>> { ... }
async post<T = unknown>(
url: string,
data: unknown,
config: PostOptions = {}
): Promise<HttpResponse<T>> { ... }Note: Line 104 has catch (error: any) outside the current scope that should also be typed as unknown or with the caught error type.
🧰 Tools
🪛 GitHub Check: Lint Codebase
[warning] 83-83:
Unexpected any. Specify a different type
[warning] 83-83:
Unexpected any. Specify a different type
[warning] 82-82:
Unexpected any. Specify a different type
[warning] 81-81:
Unexpected any. Specify a different type
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 51-51:
Unexpected any. Specify a different type
[warning] 50-50:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
In `@src/onegetjs/httpClient.ts` around lines 50 - 83, Update the HttpClient
method signatures to remove all uses of any: import got's OptionsInit and define
a generic HttpResponse<T> type (data: T, request: { res: { responseUrl?: string
} }, status: number, headers?: Record<string,string|string[]>), then change get
to async get<T = unknown>(url: string, options?: OptionsInit & { isStream?:
boolean }): Promise<HttpResponse<T>> and post to async post<T = unknown>(url:
string, data: unknown, config: OptionsInit = {}): Promise<HttpResponse<T>>;
inside get/post return types use T for response body and use proper header
typing; also change any catch clauses (e.g., catch (error: any)) to catch
(error: unknown) and narrow or rethrow appropriately.
| if (options?.isStream) { | ||
| // For streams, return the stream directly with extended timeout | ||
| const stream = this.client.stream.get(url, { | ||
| timeout: { request: 300000 } // 5 minutes for large files | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the httpClient.ts file around the specified lines
cat -n src/onegetjs/httpClient.ts | sed -n '40,70p'Repository: 1CDevFlow/onec-setup-action
Length of output: 1186
🏁 Script executed:
# Find all usages of isStream in the codebase
rg -n --type ts --type js 'isStream' -C 3 srcRepository: 1CDevFlow/onec-setup-action
Length of output: 1011
🏁 Script executed:
# Also check if there are any test files that demonstrate the usage
fd -e spec.ts -e test.ts -e .test.ts src | head -20Repository: 1CDevFlow/onec-setup-action
Length of output: 53
🏁 Script executed:
# Read the downloader.ts file to understand the full context
cat -n src/onegetjs/downloader.ts | head -100Repository: 1CDevFlow/onec-setup-action
Length of output: 3506
🏁 Script executed:
# Search for all httpClient.get calls across the codebase
rg -n 'httpClient\.get\(' srcRepository: 1CDevFlow/onec-setup-action
Length of output: 728
🏁 Script executed:
# Also check how the httpClient is instantiated and configured
rg -n 'new.*[Hh]ttp[Cc]lient\|this\.client\.get\|this\.client\.stream' src/onegetjs/httpClient.tsRepository: 1CDevFlow/onec-setup-action
Length of output: 53
🏁 Script executed:
# Check the auth provider to see how authentication is handled
cat -n src/onegetjs/auth/tokenAuthProvider.ts | head -50Repository: 1CDevFlow/onec-setup-action
Length of output: 1892
🏁 Script executed:
# Also check the base auth provider or factory
rg -n 'authenticate' src/onegetjs/auth/ -A 3Repository: 1CDevFlow/onec-setup-action
Length of output: 994
🏁 Script executed:
# Check httpClient initialization to see if there's any default interceptor/middleware
cat -n src/onegetjs/httpClient.ts | head -50Repository: 1CDevFlow/onec-setup-action
Length of output: 1717
Pass through request options in stream mode.
When options.isStream is true, the request options (including responseType and any other passed options) are silently dropped, with only the timeout being forwarded to the stream request. While the current single call site doesn't pass additional options, this creates a latent bug where future code attempting to pass headers, search parameters, or other request options will fail silently. Merge all options into the stream request, preserving the extended timeout.
- const stream = this.client.stream.get(url, {
- timeout: { request: 300000 } // 5 minutes for large files
- })
+ const { isStream, ...streamOptions } = options ?? {}
+ const stream = this.client.stream.get(url, {
+ ...streamOptions,
+ timeout: { request: 300000 } // 5 minutes for large files
+ })🤖 Prompt for AI Agents
In `@src/onegetjs/httpClient.ts` around lines 52 - 56, When options.isStream is
true, the current code calls this.client.stream.get(url, { timeout: { request:
300000 } }) and drops all other passed options; update the stream branch to
merge the incoming options object into the request while overriding/ensuring the
extended timeout (e.g., create a mergedOptions = { ...options, timeout: {
request: 300000 }, /* preserve responseType/headers/searchParams */ } and pass
mergedOptions to this.client.stream.get) so responseType, headers, searchParams,
etc. are forwarded while guaranteeing the 5-minute timeout.
|
|
||
| if (links.length === 0) { | ||
| core.error(`Don't found links for file ${file.name}`) | ||
| logger.error(`Don't found links for file ${file.name}`) |
There was a problem hiding this comment.
Fix grammar in missing-links log.
“Don't found links” is ungrammatical. Consider a clearer message.
✏️ Suggested fix
- logger.error(`Don't found links for file ${file.name}`)
+ logger.error(`No download links found for file ${file.name}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.error(`Don't found links for file ${file.name}`) | |
| logger.error(`No download links found for file ${file.name}`) |
🤖 Prompt for AI Agents
In `@src/onegetjs/index.ts` at line 56, The log message "Don't found links for
file ${file.name}" is ungrammatical; update the logger.error call (the
logger.error invocation in src/onegetjs/index.ts that references file.name) to a
clear, correct message such as "No links found for file ${file.name}" or "No
links found in file ${file.name}" so the message reads properly and includes the
file name for context.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.